Skip to content
This repository has been archived by the owner on Jul 1, 2024. It is now read-only.

Don't close PRs waiting for reviews #344

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jablko
Copy link
Contributor

@jablko jablko commented Jan 20, 2021

If the author has re-requested a review, don't close the PR as abandoned while awaiting a response.

Copy link
Contributor

@elibarzilay elibarzilay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not clear to me what this thing is doing. A test (or at least a pointer to a PR that this will change) would be good.

Specifically, it's not clear to me what happens when there's a pending review request: can the PR continue to be held open infinitely? Or maybe if I have some strong objections in a request and you ask for a re-review -- is that effectively ignoring my changereq, which would allow you to proceed?

src/pr-info.ts Outdated Show resolved Hide resolved
@jablko
Copy link
Contributor Author

jablko commented Jan 22, 2021

I added DefinitelyTyped/DefinitelyTyped#49667 (comment) as a test.

The usual timelines apply, so after re-requesting a review a PR will eventually become "Unreviewed" and end up in "Needs Maintainer Action" (it won't be held open infinitely).

You can currently hold a PR open indefinitely by repeatedly restarting the timeline (DefinitelyTyped/DefinitelyTyped#49667 (comment)), but you can't get it back into "Waiting for Code Reviews".

Re-requesting a review would effectively dismiss a changereq, but

  1. The other criteria still apply (maintainer/owner/other approval)
  2. The alternative potentiality is someone issuing a changereq and then not responding until the bot closes the PR
  3. A changereq is only good until the next push anyway

@jablko jablko force-pushed the patch-78 branch 2 times, most recently from 17adcf9 to 23ec8cc Compare January 27, 2021 15:03
If the author has re-requested a review, don't close the PR as abandoned
while awaiting a response.

* Update schema
* Add test (49667)

Co-authored-by: Eli Barzilay <[email protected]>
@elibarzilay
Copy link
Contributor

I spent more time thinking about this, and I still don't see the point.
But the more I look the more I see arguments against it.

  1. It has a fundamental complication of an implementation that just
    ignore the review, and therefore results in more code that will need
    to change. For example, having this PR end in an Unreviewed state
    is bogus, since it was reviewed, and the reviewer asked for
    changes.

  2. Looks like this was a result of @amcasey slightly misusing the system
    by submitting a changereq but ended up being happy with just
    clarifications and no actual changes. Even if this wasn't the case,
    since it's such an edge case, you could have avoided the closure by
    just pushing an update, which would achieve a similar result. (It's
    a hack, but it's a rare case to begin with.) Such an update would
    make the bot ask for an updated review.

  3. Ignoring the contents, this PR actually allows potentially bad
    behavior: A does a changereq for your PR and B approves it, you
    dismiss the changereq by asking for a re-review, and now you can
    merge it since now it has just the approval. The bad thing here is
    that you could ignore changereqs this way.

    Again, it is possible to do this anyway, by pushing a dummy update,
    nut since you're already updating the PR, you'd usually try to adjust
    code according to the request anyway. Also, such an update would
    delay closing the PR in a very similar (except that then an
    "Unreviewed" label would actually make sense).

  4. Finally, without this, and even without a dummy update to game the
    system, you'd normally ping the reviewer and note that you replied to
    the questions and no change was needed, and that would reset the
    activity clock, and encourage the reviewer to have a second look and
    either stick to the request or change it to passing.

Given all of this, I now think that this change should not get merged.

@elibarzilay
Copy link
Contributor

(Pushed a rebased + squashed version since I did it anyway.)

@jablko
Copy link
Contributor Author

jablko commented Jan 31, 2021

Thanks for thinking carefully about this.

Yes (to point 4), if you reply to a changereq without pushing any changes you'd normally ping the reviewer and that would reset the activity clock, however it doesn't move it to Waiting for Code Reviews/Needs Maintainer Review (where a response is more likely). If the reviewer doesn't reply back it'll get an Abandoned label and eventually get closed.

With this change it does move columns. If the reviewer doesn't reply it gets an Unreviewed label and eventually ends up in Needs Maintainer Action, which I still think is an improvement?

Yes also (to point 3), this change does allow bad behavior, although the bot already allows some bad behavior.

I'm not sure it's the goal to be ironclad, as long as bad behavior is transparently antisocial to a human? In this case if you had an approval and a changereq, re-requested a review from the changereqer, and then merged the PR without giving them a chance to respond, that would be transparent to that reviewer (they'd be notified of the review request). I'm not sure it's the bot's role to police this? If however the reviewer ignored the request then I think it's reasonable to proceed based on other approvals/checks.

@amcasey's use of the system seems like a natural one, just one that isn't currently supported (because the bot would've closed the PR), and this PR resolves that.

I think re-requesting a review is preferable to a dummy push because:

  • I'm not sure contributors know to do that in this situation
  • I'm not sure the bot should assume a push resolves a changereq (and a reply does not)

However I do take your point that this case is rare.

I'd still say if you re-request a review (after a reviewer asked for changes), Unreviewed is less bogus than Abandoned? I'm not sure any other code changes are needed?

@elibarzilay
Copy link
Contributor

Just some quick replies:

  1. Normally people would ping the reviewer in a comment anyway. If it gets closed, there's also the simple option of re-opening it.

  2. It shouldn't be robust against malicious behavior --- I'm more concerned about unintentionally bad behavior: I request a review from A, B authorize it, I'm so happy I quickly merge it, A comes back and complains about ignoring that review.

@jablko
Copy link
Contributor Author

jablko commented Feb 2, 2021

Do you have to be a maintainer to reopen a PR once the bot closes it?

@elibarzilay elibarzilay force-pushed the master branch 3 times, most recently from 1d39a68 to 9e931d1 Compare July 7, 2021 15:39
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants